send webhook when task or workflow run is canceled #1374
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to ac4a400 in 25 minutes and 27 seconds
More details
- Looked at
154lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. skyvern/forge/agent.py:1546
- Draft comment:
The change to acceptlast_stepasStep | Noneinexecute_task_webhookis appropriate and consistent with its usage. - Reason this comment was not posted:
Confidence changes required:0%
The change inexecute_task_webhookmethod to acceptlast_stepasStep | Noneis appropriate, as it allows for flexibility when the last step might not be available. This change is consistent with the usage in the codebase.
2. skyvern/forge/sdk/workflow/service.py:888
- Draft comment:
The change to acceptapi_keyasstr | Noneinexecute_workflow_webhookis appropriate and consistent with its usage. - Reason this comment was not posted:
Confidence changes required:0%
The change inexecute_workflow_webhookmethod to acceptapi_keyasstr | Noneis appropriate, as it allows for cases where the API key might not be provided. This change is consistent with the usage in the codebase.
3. skyvern/forge/sdk/routes/agent_protocol.py:287
- Draft comment:
The addition ofx_api_keyas a header incancel_taskis appropriate for passing API keys for authentication. - Reason this comment was not posted:
Confidence changes required:0%
The change incancel_taskto includex_api_keyas a header is consistent with the need to pass API keys for authentication and is used correctly in the function.
4. skyvern/forge/sdk/routes/agent_protocol.py:308
- Draft comment:
The addition ofx_api_keyas a header incancel_workflow_runis appropriate for passing API keys for authentication. - Reason this comment was not posted:
Confidence changes required:0%
The change incancel_workflow_runto includex_api_keyas a header is consistent with the need to pass API keys for authentication and is used correctly in the function.
Workflow ID: wflow_mRaU7xEO2B9PxSqw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
LawyZheng
reviewed
Dec 13, 2024
| @@ -292,16 +293,28 @@ async def cancel_task( | |||
| status_code=status.HTTP_404_NOT_FOUND, | |||
| detail=f"Task not found {task_id}", | |||
| ) | |||
| await app.agent.update_task(task_obj, status=TaskStatus.canceled) | |||
| task = await app.agent.update_task(task_obj, status=TaskStatus.canceled) | |||
There was a problem hiding this comment.
shall we just only send the webhook when task/workflow cancellation is finished? any specific reason why we must send the webhook in the API call?
There was a problem hiding this comment.
if user call multiple times to the cancle endpoint. he will receive multiple times the webhook without any task/workflow being cancelled 🤔
There was a problem hiding this comment.
users can also cancel tasks that are stuck. our worker won't really send a webhook if we don't send a webhook to them in the API endpoint.
There was a problem hiding this comment.
i think it's okay that we send two cancel task webhook.
LawyZheng
reviewed
Dec 13, 2024
Comment on lines
+310
to
+317
| workflow_run = await app.DATABASE.get_workflow_run(workflow_run_id=workflow_run_id) | ||
| if not workflow_run: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail=f"Workflow run not found {workflow_run_id}", | ||
| ) | ||
| await app.WORKFLOW_SERVICE.mark_workflow_run_as_canceled(workflow_run_id) | ||
| await app.WORKFLOW_SERVICE.execute_workflow_webhook(workflow_run, api_key=x_api_key) |
wintonzheng
deleted the
shu/send_webhook_when_task.workflow_is_canceled
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Send webhooks when tasks or workflow runs are canceled by updating
execute_step(),execute_workflow(), and related functions to handle webhook execution.need_call_webhook=Trueinexecute_step()inagent.pyandexecute_workflow()inservice.py.cancel_task()andcancel_workflow_run()inagent_protocol.pyto includex_api_keyfor webhook execution.execute_task_webhook()inagent.pyto acceptlast_stepasStep | None.execute_workflow_webhook()inservice.pyto handle webhook execution for workflow runs.service.pyfor webhook execution.This description was created by
for ac4a400. It will automatically update as commits are pushed.